merge dev to main (v3.0.0-alpha.21) #157
Conversation
WalkthroughIntroduces a BaseCrudDialect.fieldRef method and refactors runtime code to use it across filtering, ordering, selection, and grouping. Updates aggregation utilities to support more operators and enforce exhaustiveness. Expands computed-field tests. Bumps package versions from 3.0.0-alpha.20 to 3.0.0-alpha.21 across multiple packages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR merges development changes to main, incrementing package versions from 3.0.0-alpha.20 to 3.0.0-alpha.21 across the monorepo. The changes include version bumps, new test cases for computed fields, and improvements to field reference handling in database query operations.
- Version increment across all packages from alpha.20 to alpha.21
- Enhanced computed field testing with filtering, ordering, and aggregation operations
- Refactored field reference building to use a centralized
fieldRefmethod in dialect classes
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Multiple package.json files | Version bumps from 3.0.0-alpha.20 to 3.0.0-alpha.21 |
| packages/runtime/test/client-api/computed-fields.test.ts | Added comprehensive tests for computed field operations |
| packages/runtime/src/client/query-utils.ts | Simplified aggregate function signature |
| packages/runtime/src/client/crud/operations/group-by.ts | Refactored to use centralized field reference method |
| packages/runtime/src/client/crud/operations/base.ts | Updated to use dialect's fieldRef method |
| packages/runtime/src/client/crud/dialects/*.ts | Implemented fieldRef method in dialect classes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #157: merge dev to main (v3.0.0-alpha.20 → v3.0.0-alpha.21) Todo List:
Review SummaryThis PR merges development changes from alpha.20 to alpha.21, including version bumps across all packages and important improvements to computed field handling and field reference management. Code Quality & Best Practices ✅Strengths:
Performance Considerations ✅Positive aspects:
No performance concerns identified. Security Assessment ✅Security posture:
Test Coverage Assessment ✅Strong test additions:
Potential Issues & RecommendationsMinor Concerns:
Code Quality Observations:Excellent patterns:
Technical ArchitectureThe refactoring maintains ZenStack's architectural principles:
Recommendation: ✅ APPROVEThis is a well-executed maintenance release that:
The version bump to alpha.21 is appropriate for these improvements. The changes are ready for merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/runtime/src/client/query-utils.ts (1)
321-329: Add explicit return type to stabilize API surfaceGood addition of _avg/_min/_max and exhaustive handling. To avoid unintended type drift and preserve the function’s public contract, add an explicit return type (it used to be Expression).
Apply this diff:
-export function aggregate(eb: ExpressionBuilder<any, any>, expr: Expression<any>, op: AGGREGATE_OPERATORS) { +export function aggregate( + eb: ExpressionBuilder<any, any>, + expr: Expression<any>, + op: AGGREGATE_OPERATORS, +): Expression<any> {packages/runtime/src/client/crud/dialects/base.ts (2)
736-747: Remove redundant _count orderBy branch (now unreachable)The general aggregation branch above already handles '_count' (and continues the loop). This specific case is now dead code and can be removed to reduce confusion.
Apply this diff to remove the special-casing:
- switch (field) { - case '_count': { - invariant(value && typeof value === 'object', 'invalid orderBy value for field "_count"'); - for (const [k, v] of Object.entries<string>(value)) { - invariant(v === 'asc' || v === 'desc', `invalid orderBy value for field "${field}"`); - result = result.orderBy( - (eb) => eb.fn.count(this.fieldRef(model, k, eb, modelAlias)), - sql.raw(this.negateSort(v, negated)), - ); - } - continue; - } - default: - break; - } + // no-op: aggregations handled above
755-770: Avoid building expressions with a detached expressionBuilder in orderByPrefer the callback form of orderBy to build expressions with the query’s own ExpressionBuilder. This keeps expression construction consistent and avoids unexpected builder scoping issues.
Apply this diff:
- const fieldRef = this.fieldRef(model, field, expressionBuilder(), modelAlias); - if (value === 'asc' || value === 'desc') { - result = result.orderBy(fieldRef, this.negateSort(value, negated)); - } else if ( + if (value === 'asc' || value === 'desc') { + result = result.orderBy( + (eb) => this.fieldRef(model, field, eb, modelAlias), + this.negateSort(value, negated), + ); + } else if ( value && typeof value === 'object' && 'nulls' in value && 'sort' in value && (value.sort === 'asc' || value.sort === 'desc') && (value.nulls === 'first' || value.nulls === 'last') ) { result = result.orderBy( - fieldRef, + (eb) => this.fieldRef(model, field, eb, modelAlias), sql.raw(`${this.negateSort(value.sort, negated)} nulls ${value.nulls}`), ); }And remove the now-unused import:
- import { expressionBuilder, sql, type SelectQueryBuilder } from 'kysely'; + import { sql, type SelectQueryBuilder } from 'kysely';packages/runtime/test/client-api/computed-fields.test.ts (1)
40-90: Great coverage for computed fields; consider adding numeric aggregationsNice additions covering where/orderBy/negative take/aggregate/groupBy using a computed field. To fully exercise the expanded aggregations, consider adding tests for _avg/_sum/_min on a numeric computed field (e.g., nameLength: Int @computed as LENGTH(name)) and ordering by those aggregates.
I can add a minimal numeric-computed-field setup and corresponding tests if you’d like.
packages/runtime/src/client/crud/dialects/sqlite.ts (1)
202-202: Unnecessary type casting to ArgsType.The type cast
as ArgsTypeon this line appears to be redundant sincethis.fieldRefshould already return a type compatible withArgsType. The same pattern on line 173 doesn't use this cast.Consider removing the unnecessary type cast for consistency:
-return [sql.lit(field), this.fieldRef(relationModel, field, eb) as ArgsType]; +return [sql.lit(field), this.fieldRef(relationModel, field, eb)];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/eslint-config/package.json(1 hunks)packages/ide/vscode/package.json(1 hunks)packages/language/package.json(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/crud/dialects/base.ts(8 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(2 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(2 hunks)packages/runtime/src/client/crud/operations/base.ts(3 hunks)packages/runtime/src/client/crud/operations/group-by.ts(5 hunks)packages/runtime/src/client/query-utils.ts(1 hunks)packages/runtime/test/client-api/computed-fields.test.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/typescript-config/package.json(1 hunks)packages/vitest-config/package.json(1 hunks)packages/zod/package.json(1 hunks)samples/blog/package.json(1 hunks)tests/e2e/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/eslint-config/package.jsonpackages/language/package.jsonpackages/ide/vscode/package.jsonpackages/testtools/package.jsonpackages/common-helpers/package.jsonsamples/blog/package.jsonpackages/typescript-config/package.jsonpackages/zod/package.jsonpackages/vitest-config/package.jsonpackages/cli/package.jsonpackages/tanstack-query/package.jsonpackages/runtime/src/client/crud/dialects/sqlite.tspackages/sdk/package.jsonpackages/runtime/package.jsontests/e2e/package.jsonpackages/runtime/src/client/crud/operations/base.tspackages/create-zenstack/package.jsonpackages/dialects/sql.js/package.jsonpackages/runtime/src/client/query-utils.tspackages/runtime/src/client/crud/dialects/postgresql.tspackages/runtime/src/client/crud/dialects/base.tspackages/runtime/test/client-api/computed-fields.test.tspackages/runtime/src/client/crud/operations/group-by.ts
tests/e2e/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
E2E tests are in
tests/e2e/directory
Files:
tests/e2e/package.json
🧠 Learnings (1)
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: `zenstack generate` compiles ZModel to TypeScript schema (`schema.ts`)
Applied to files:
packages/language/package.json
🧬 Code Graph Analysis (4)
packages/runtime/src/client/crud/operations/base.ts (1)
packages/runtime/src/client/crud/dialects/base.ts (1)
fieldRef(996-998)
packages/runtime/src/client/query-utils.ts (2)
packages/sdk/src/schema/expression.ts (1)
Expression(1-10)packages/runtime/src/client/constants.ts (2)
AGGREGATE_OPERATORS(29-29)AGGREGATE_OPERATORS(30-30)
packages/runtime/src/client/crud/dialects/base.ts (2)
packages/runtime/src/client/query-utils.ts (2)
aggregate(321-329)buildFieldRef(157-179)packages/runtime/src/client/constants.ts (2)
AGGREGATE_OPERATORS(29-29)AGGREGATE_OPERATORS(30-30)
packages/runtime/src/client/crud/operations/group-by.ts (2)
packages/runtime/src/client/crud/dialects/base.ts (1)
fieldRef(996-998)packages/runtime/src/client/query-utils.ts (1)
aggregate(321-329)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x)
- GitHub Check: claude-review
🔇 Additional comments (35)
packages/create-zenstack/package.json (1)
3-3: All package.json versions are consistent at 3.0.0-alpha.21Verified via script—no mismatches found across all packages. Metadata-only bump; no further changes needed.
packages/sdk/package.json (1)
3-3: LGTM: SDK manifest version bumped as part of coordinated releaseLooks consistent with the monorepo-wide alpha.21 bump; no other manifest changes.
packages/language/package.json (1)
4-4: LGTM: language package version updatedMetadata-only change; build and export config remain unchanged.
packages/zod/package.json (1)
3-3: LGTM: zod package version bumpPrivate package, metadata-only; aligns with the release.
packages/cli/package.json (1)
6-6: LGTM: CLI package version bumpNo behavioral changes; aligns with other packages for alpha.21 release.
packages/tanstack-query/package.json (1)
3-3: LGTM: Version bump is consistent with the release train.
No other changes in this manifest; dependency and peer ranges remain intact.packages/eslint-config/package.json (1)
3-3: LGTM: Metadata-only version bump.
No config or dependency changes; safe to merge.packages/common-helpers/package.json (1)
3-3: LGTM: Version updated to alpha.21.
No export or script changes; aligns with the monorepo rollout.package.json (2)
3-3: LGTM: Root version bump to alpha.21.
Matches the repo-wide version update.
3-3: No stale references to 3.0.0-alpha.20 remain
Ran the verification script and confirmed:
rgfound zero occurrences of3.0.0-alpha.20- All
package.jsonfiles list"version": "3.0.0-alpha.21"samples/blog/package.json (1)
3-3: LGTM: Sample app version bumped.
No impact to scripts or workspace dependencies.packages/ide/vscode/package.json (1)
4-4: Version bump looks goodSemver prerelease bump only; no other changes. Safe to merge.
packages/typescript-config/package.json (1)
3-3: Version bump looks goodMetadata-only change; no behavioral impact.
packages/testtools/package.json (1)
3-3: Version bump looks goodNo functional changes; consistent with repo-wide bump.
packages/runtime/package.json (2)
3-3: Version bump looks goodAligned with the runtime changes introduced elsewhere in the PR.
3-3: Repo-wide versions and workspace dependencies are consistentAll package.json files have been updated to 3.0.0-alpha.21, and no @zenstackhq/* dependencies are hard-pinned to a non-workspace version.
tests/e2e/package.json (1)
3-3: Version bump looks goodE2E package version aligned with the monorepo bump.
packages/vitest-config/package.json (1)
4-4: Version bump aligns with the release trainMonorepo version bump to 3.0.0-alpha.21 looks consistent and isolated. No concerns.
packages/dialects/sql.js/package.json (1)
3-3: Version bump only; safe changePackage version updated to 3.0.0-alpha.21 with no other alterations. Looks good.
packages/runtime/src/client/crud/dialects/base.ts (4)
98-104: Centralized fieldRef usage in filters is correctSwitching to this.fieldRef respects originModel and unlocks computed-field support across filter paths. Good improvement.
728-731: OrderBy aggregation now properly leverages fieldRefUsing aggregate(eb, this.fieldRef(...)) ensures ordering by computed or aliased fields works across dialects. Looks good.
869-869: Computed-field selection via fieldRef is correctSelecting computed fields with this.fieldRef(...).as(field) ensures parity with non-computed fields and honors aliases. Good change.
996-999: New fieldRef helper centralizes field reference constructionExposing fieldRef on the dialect and delegating to buildFieldRef improves consistency and encapsulation. Good abstraction.
packages/runtime/src/client/crud/operations/base.ts (3)
189-189: LGTM! Consistent use of dialect.fieldRef for distinct field references.The change correctly uses the new centralized
dialect.fieldRefAPI for building field references in the distinct field selection, aligning with the broader refactoring pattern across the codebase.
1267-1267: LGTM! Proper migration to dialect.fieldRef for incremental updates.The change correctly uses
dialect.fieldReffor building field references in incremental update operations, maintaining consistency with the new API pattern.
1290-1290: LGTM! Consistent use of dialect.fieldRef for scalar list updates.The change properly uses
dialect.fieldReffor building field references in scalar list update operations, aligning with the centralized field reference API.packages/runtime/src/client/crud/dialects/sqlite.ts (1)
173-173: LGTM! Consistent migration to fieldRef in SQLite dialect.The change correctly replaces
buildFieldRefwiththis.fieldReffor scalar field references in the SQLite dialect, maintaining consistency with the centralized field reference API introduced in the base dialect.packages/runtime/src/client/crud/dialects/postgresql.ts (2)
229-229: LGTM! Proper migration to fieldRef in PostgreSQL dialect.The change correctly replaces
buildFieldRefwiththis.fieldReffor scalar field references, maintaining consistency with the centralized field reference API.
252-252: LGTM! Consistent use of fieldRef for field selection.The change properly uses
this.fieldReffor building field references in the select clause, aligning with the new API pattern.packages/runtime/src/client/crud/operations/group-by.ts (6)
1-1: LGTM! Appropriate imports for the new field reference pattern.The addition of
expressionBuilderandaggregateimports properly supports the new dialect-driven field reference approach.Also applies to: 4-4
47-47: LGTM! Well-designed local helper for field references.The
fieldRefhelper function provides a clean abstraction for referencing fields from the subquery alias$sub, properly utilizing the new dialect API.
51-51: LGTM! Consistent use of fieldRef for groupBy clause.The change correctly maps all groupBy fields through the
fieldRefhelper, ensuring consistent field referencing from the subquery.
64-64: LGTM! Proper use of fieldRef for field selection.The change correctly uses the
fieldRefhelper for selecting grouped-by fields, maintaining consistency with the new field reference pattern.
82-82: LGTM! Consistent use of fieldRef for count operations.The change properly uses the
fieldRefhelper for per-field count operations, aligning with the new API pattern.
97-97: LGTM! Excellent unification of aggregation operations.The change effectively consolidates all aggregation operations (
_sum,_avg,_max,_min) through the newaggregateutility function, improving code maintainability and ensuring exhaustive handling of aggregate operators.
Summary by CodeRabbit